Upgrading to use aws sdk 1.9.291 #3353
Conversation
|
This pull request has been linked to Shortcut Story #18651: Update AWS SDK. |
…ng CI failures on shutdown
dhoke4tdb
force-pushed
the
dlh/sc-18651-upg-aws-sdk
branch
from
7e3af32 to
31c17bc
Compare
There was a problem hiding this comment.
Some comments inline... Something seems wrong here with the number of explicit targets needed -- are those not being set by the SDK when we target S3? Or are we doing something wrong when creating our "shadow" targets such that the transitive requirements are not pulled in?
cmake/Modules/FindAWSSDK_EP.cmake
Outdated
| URL_HASH SHA1=e32a53a01c75ca7fdfe9feed9c5bbcedd98708e3 | ||
| GIT_REPOSITORY "https://github.com/aws/aws-sdk-cpp" | ||
| GIT_TAG 1.9.291 | ||
| GIT_SUBMODULES_RECURSE ON #TBD: Seems 'ON' may be default, is this needed? Is this correct? |
There was a problem hiding this comment.
👍 Yes, needed - this does git clone --recurse-submodules ... which they specify as required in the docs, b/c some of the dependencies are now submodules and won't be populated w/out this option.
cmake/Modules/FindAWSSDK_EP.cmake
Outdated
| aws-checksums | ||
| aws-c-sdkutils | ||
| aws-crt-cpp | ||
| #xAWSSDK::aws-c-s3 |
cmake/Modules/FindAWSSDK_EP.cmake
Outdated
| aws-cpp-sdk-s3 | ||
| aws-cpp-sdk-core | ||
| aws-crt-cpp | ||
| #AWSSDK::aws-c-auth |
There was a problem hiding this comment.
Remove cruft, if not necessary?
| thread_options.join_strategy = AWS_TJS_MANAGED; | ||
|
|
||
| + /* | ||
| + * tiledb encountered issue similar to https://github.com/huggingface/datasets/issues/3310#issuecomment-1155285967. |
There was a problem hiding this comment.
The underlying issue is here: https://github.com/aws/aws-sdk-cpp/issues/1809
Please link that too, and add a short summary so that we have some context to start from even if the linked comment disappears.
| + * tiledb encountered issue similar to https://github.com/huggingface/datasets/issues/3310#issuecomment-1155285967. | ||
| + */ | ||
| +#if 1 | ||
| + aws_thread_launch(&cleanup_thread, s_event_loop_destroy_async_thread_fn, el_group, &thread_options); |
There was a problem hiding this comment.
If you are able to easily reproduce the issue mentioned above, could you please check GetLastError here and dump it out. That might be useful to pushing AWS on the issue...
There was a problem hiding this comment.
Also a stack trace - I believe the problem is that they are trying to launch a thread after process exit has started, which fails (even on POSIX). See
- Aws::Crt::Io::ClientBootstrap destructor may launch thread at process exit, and crash aws/aws-sdk-cpp#1809 (comment)
- and stack trace from elsewhere, confirming above (
ExitProcessin backtrace): ARROW-12626: [C++] Support toolchain xsimd, update toolchain version to version 8.1.0 apache/arrow#13244 (comment)
With previous version of sdk we used, findaws module was using and adding (five) additional libs. When I tried to use that alone (which I would think is its purpose), it did not seem to provide all needed dependencies, and the ones it did provide did not seem to be in correct order for linkage (even though comments I saw somewhere suggested that was a benefit of using it.) Trying to use it similarly to prev version ('adding' to result), IIRC I was hitting linkage (ordering) issues. So, what is here now is the first form I arrived at that supported successful build. I'll explore a bit more again to try to see what I can alter/eliminate, |
…hat seem to be needed in addition
…owing alternate failing approaches tried
…ently needed by tiledb
Upgrading to use aws sdk 1.9.291, builds locally win/nix, may be building more than necessary, checkpoint how does it do with CI
TYPE: IMPROVEMENT
DESC: Upgrading to use aws sdk 1.9.291